Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert oid expiration timestamp to datetime.datetime #801

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

droctothorpe
Copy link
Contributor

After enabling refresh tokens on our oidc backend (shout out to @harrisonfritz), dask-kubernetes still failed. I narrowed down the problem to the fact that parse_rfc3339 is not equipped to handle an expiration_timestamp in epoch string format, which is what the OIDC protocol spec specifies (shout out to @DWSR for pointing us to the relevant docs):

exp
REQUIRED. Expiration time on or after which the ID Token MUST NOT be accepted for processing. The processing of this parameter requires that the current date/time MUST be before the expiration date/time listed in the value. Implementers MAY provide for some small leeway, usually no more than a few minutes, to account for clock skew. Its value is a JSON number representing the number of seconds from 1970-01-01T0:0:0Z as measured in UTC until the date/time. See RFC 3339 [RFC3339] for details regarding date/times in general and UTC in particular.

Emphasis mine. I manually validated that this works on our end. Not sure why no one has run into it before. Perhaps the oidc auth path is not widely adopted or most people are invoking it in-cluster (where the service account token is consumed).

@jacobtomlinson
Copy link
Member

Thanks for raising this. The code you've changed here is actually part of the classic code that is in the process of being stripped out so this will be removed again in the very near future. However, given that it's such a small fix I'm happy to merge it for now.

Not sure why no one has run into it before. Perhaps the oidc auth path is not widely adopted or most people are invoking it in-cluster (where the service account token is consumed).

I think you're right.

@jacobtomlinson jacobtomlinson merged commit 00dca20 into dask:main Aug 15, 2023
18 checks passed
@droctothorpe
Copy link
Contributor Author

Thanks, @jacobtomlinson! This clears the last obstacle for us to adopt the operator pattern. Really appreciate the merge.

@droctothorpe droctothorpe deleted the datetime branch August 15, 2023 12:47
@jacobtomlinson
Copy link
Member

That's great! I'll get a patch release out with this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants